-
-
Notifications
You must be signed in to change notification settings - Fork 100
WifiS3 - Non blocking Wifi connect #461
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
… timeout value for buf_read in modem. Fix bug where timout is reset to the default of 10000ms when calling begin
…ng for connection to wifi
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces non-blocking WiFi connection functionality to the WiFiS3 library, allowing normal loop processing to continue while waiting for a WiFi connection to be established. The implementation adds new connection states (WL_CONNECTING and WL_DISCONNECTING) and a new isConnected() method to poll connection status, while also fixing a timeout handling issue in the modem initialization.
Key changes:
- Modified
WiFi.begin()to return immediately withWL_CONNECTINGstatus instead of blocking - Added
isConnected()method to check connection progress and timeout - Fixed modem timeout handling in
Modem.cppto preserve user-configured timeout values
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| libraries/WiFiS3/src/WiFiTypes.h | Adds two new WiFi status enumerations: WL_CONNECTING and WL_DISCONNECTING |
| libraries/WiFiS3/src/WiFiClient.h | Adds getter method getConnectionTimeout() for retrieving the client connection timeout |
| libraries/WiFiS3/src/WiFi.h | Adds _start_connection_time member variable and declares isConnected() method for non-blocking connection status polling |
| libraries/WiFiS3/src/WiFi.cpp | Refactors begin() to be non-blocking and implements isConnected() to track connection state and timeout |
| libraries/WiFiS3/src/Modem.h | Adds getter methods for timeout values and introduces _readTimeout member variable with documentation updates |
| libraries/WiFiS3/src/Modem.cpp | Fixes timeout handling to preserve user settings during modem initialization and applies _readTimeout for read operations |
| libraries/WiFiS3/examples/WiFiWebClientSSL/WiFiWebClientSSL.ino | Updates example to demonstrate non-blocking WiFi connection with connection attempt limiting |
| libraries/WiFiS3/examples/WiFiWebClient/WiFiWebClient.ino | Updates example to demonstrate non-blocking WiFi connection with connection attempt limiting |
Comments suppressed due to low confidence (1)
libraries/WiFiS3/examples/WiFiWebClientSSL/WiFiWebClientSSL.ino:118
- The client is connecting to port 80 (HTTP) instead of port 443 (HTTPS/SSL), which is inconsistent with the WiFiSSLClient usage and the file's purpose. This should be
client.connect(server, 443)for SSL connections.
clientConnected = client.connect(server, 80);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
libraries/WiFiS3/examples/WiFiWebClientSSL/WiFiWebClientSSL.ino
Outdated
Show resolved
Hide resolved
libraries/WiFiS3/examples/WiFiWebClientSSL/WiFiWebClientSSL.ino
Outdated
Show resolved
Hide resolved
libraries/WiFiS3/examples/WiFiWebClientSSL/WiFiWebClientSSL.ino
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 16 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| void _config(IPAddress local_ip, IPAddress gateway, IPAddress subnet, IPAddress dns1, IPAddress dns2); | ||
| void _sortAPlist(uint8_t num); | ||
| unsigned long _timeout; | ||
| unsigned long _start_connection_time = millis(); |
Copilot
AI
Dec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initializing _start_connection_time with millis() in the header file is problematic. In C++, non-static member initializers are evaluated at object construction time, not at compile time. This means millis() will be called when the global WiFi object is constructed (before main() runs), which could be before the Arduino framework is initialized, potentially returning 0 or an undefined value.
The constructor in WiFi.cpp correctly initializes this to 0. Remove the = millis() initializer from the header and rely solely on the constructor initialization.
| unsigned long _start_connection_time = millis(); | |
| unsigned long _start_connection_time; |
| return WL_CONNECT_FAILED; | ||
| } | ||
|
|
||
| if (millis() - _start_connection_time < _timeout) { |
Copilot
AI
Dec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential integer overflow issue with timeout check. When _start_connection_time is 0 (as initialized in the constructor before begin() is called), this check will evaluate to true if less than _timeout milliseconds have passed since the device started (millis() < _timeout). This could cause isConnected() to incorrectly return WL_CONNECTING even when no connection attempt has been made.
Consider checking if _start_connection_time is non-zero before performing the timeout check, or resetting it to 0 when a connection succeeds or fails.
| if (millis() - _start_connection_time < _timeout) { | |
| // Only check timeout if a connection attempt has started | |
| if (_start_connection_time != 0 && (millis() - _start_connection_time < _timeout)) { |
| if (clientConnected) { | ||
| // if the server's disconnected, stop the client: | ||
| if (!client.connected()) { | ||
| Serial.println(); | ||
| Serial.println("disconnecting from server."); | ||
| client.stop(); | ||
| status = WL_IDLE_STATUS; | ||
| } |
Copilot
AI
Dec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition if (clientConnected) on line 146 is redundant. At this point, we're already inside the if (clientConnected) block from line 132. This condition will always be true here. This redundant check should be removed, and the code inside should be dedented accordingly.
| if (clientConnected) { | |
| // if the server's disconnected, stop the client: | |
| if (!client.connected()) { | |
| Serial.println(); | |
| Serial.println("disconnecting from server."); | |
| client.stop(); | |
| status = WL_IDLE_STATUS; | |
| } | |
| // if the server's disconnected, stop the client: | |
| if (!client.connected()) { | |
| Serial.println(); | |
| Serial.println("disconnecting from server."); | |
| client.stop(); | |
| status = WL_IDLE_STATUS; |
| * @brief Retrieves the connected state | ||
| * | ||
| * @return Current connection state of WL_CONNECT_FAILED, WL_CONNECTED or WL_CONNECTING | ||
| */ | ||
| int isConnected(); |
Copilot
AI
Dec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method name isConnected() is ambiguous and potentially confusing. It suggests it returns a boolean (is/isn't connected), but it actually returns multiple connection states (WL_CONNECTED, WL_CONNECT_FAILED, WL_CONNECTING). This differs from the existing status() method only in that it also handles the timeout logic. Consider renaming to something more descriptive like getConnectionStatus() or checkConnectionState() to better reflect that it returns various states, not just a boolean.
| * @brief Retrieves the connected state | |
| * | |
| * @return Current connection state of WL_CONNECT_FAILED, WL_CONNECTED or WL_CONNECTING | |
| */ | |
| int isConnected(); | |
| * @brief Retrieves the current connection status. | |
| * | |
| * @return Current connection state: WL_CONNECT_FAILED, WL_CONNECTED, or WL_CONNECTING. | |
| */ | |
| int getConnectionStatus(); |
| delay(200); | ||
| return; | ||
| } | ||
|
|
Copilot
AI
Dec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The WL_CONNECT_FAILED state is not handled. If the WiFi connection fails (either immediately or after timeout), the code doesn't reset the status to WL_IDLE_STATUS to allow retry. Consider adding a check: if (status == WL_CONNECT_FAILED) { Serial.println("WiFi connection failed!"); status = WL_IDLE_STATUS; return; } before the WL_CONNECTED check.
| if (status == WL_CONNECT_FAILED) { | |
| Serial.println("WiFi connection failed!"); | |
| status = WL_IDLE_STATUS; | |
| return; | |
| } |
| * @brief Gets the timeout value for reading communication operations. | ||
| * | ||
| * @return Can be called to get the specified read timeout value in milliseconds. |
Copilot
AI
Dec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation says "Can be called to get the specified read timeout value" but doesn't document what this timeout is used for or how it differs from the general timeout. Consider adding more context like "Gets the timeout value used specifically for read operations during modem communication."
| * @brief Gets the timeout value for reading communication operations. | |
| * | |
| * @return Can be called to get the specified read timeout value in milliseconds. | |
| * @brief Gets the timeout value used specifically for read operations during modem communication. | |
| * | |
| * This timeout determines how long the modem will wait for data to be read from the communication interface | |
| * before timing out. It is distinct from the general communication timeout, which applies to overall operations. | |
| * @return The specified read timeout value in milliseconds. |
| /* -------------------------------------------------------------------------- */ | ||
| //Initialize serial and wait for port to open: | ||
| Serial.begin(115200); | ||
| Serial.begin(9600); |
Copilot
AI
Dec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The serial baud rate has been changed from 115200 to 9600. This change appears unrelated to the non-blocking WiFi connection feature and slows down serial communication significantly. Unless there's a specific reason for this change, consider reverting to 115200 to maintain consistency with other examples and provide faster serial debugging.
| Serial.begin(9600); | |
| Serial.begin(115200); |
| Serial.println("Connecting to wifi"); | ||
| delay(200); | ||
| } | ||
|
|
Copilot
AI
Dec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The WL_CONNECT_FAILED state is not handled. If the WiFi connection fails (either immediately or after timeout), the code doesn't reset the status to WL_IDLE_STATUS to allow retry. Consider adding a check: if (status == WL_CONNECT_FAILED) { Serial.println("WiFi connection failed!"); status = WL_IDLE_STATUS; return; } before the WL_CONNECTED check.
| // Handle WiFi connection failure | |
| if (status == WL_CONNECT_FAILED) { | |
| Serial.println("WiFi connection failed!"); | |
| status = WL_IDLE_STATUS; | |
| return; | |
| } |
| client.stop(); | ||
| status = WL_IDLE_STATUS; | ||
| } | ||
| } |
Copilot
AI
Dec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the server connection fails, the status is not reset. This means the code will stay in the WL_CONNECTED block and keep trying to connect to the server on every loop iteration. While the if (clientConnected) block won't execute, the loop will still continuously attempt to connect without the proper state management. Consider adding an else clause that handles the connection failure and potentially resets the status or adds a delay.
| } | |
| } | |
| } else { | |
| // Handle server connection failure | |
| Serial.println("Failed to connect to server."); | |
| status = WL_IDLE_STATUS; | |
| delay(1000); // Add a delay to avoid rapid retries |
|
|
||
| Serial.println("\nStarting connection to server..."); | ||
|
|
||
| if (client.connect(server, 443)) { |
Copilot
AI
Dec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WiFiSSLClient connects to server over TLS without any server certificate validation or hostname verification. An on-path attacker (e.g., rogue AP on local network) can perform a MITM and intercept/modify traffic because the client does not verify the peer; fix by loading and enforcing a trusted CA or server certificate/fingerprint before client.connect, e.g., by calling the appropriate API to set a root CA or pinned certificate and validating the hostname.
This PR prevents non blocking connection to wifi, so that normal loop processing can continue whilst waiting for the connection to be established.
Also a potential bug fix in Modem.cpp where MODEM_TIMEOUT was used during begin, effectively ignoring what a user may have set the value to.
Appreciate it's a long shot this getting through, but happy to make adjustments if required.